fix: clean up classes on glare card dismiss #668
Conversation
WalkthroughReplaces local helper-based modal toggling with Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant GlareCard as GlareCard.vue
participant Vue as Vue Reactivity
participant DOM as Document/Body
rect rgba(220,240,255,0.35)
Note over GlareCard: Component setup
GlareCard->>Vue: defineModel({ default: false })
Vue-->>GlareCard: model = false
GlareCard->>Vue: watch(model, immediate: true)
Vue->>DOM: add/remove `overflow-lock` based on model
end
rect rgba(230,255,230,0.35)
Note over User,GlareCard: User clicks overlay
User->>GlareCard: click overlay
GlareCard->>GlareCard: model = false
Vue->>DOM: remove `overflow-lock`
end
rect rgba(255,235,220,0.35)
Note over GlareCard: Teardown
GlareCard->>Vue: onBeforeUnmount
Vue->>DOM: ensure `overflow-lock` removed
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/.vitepress/theme/glare-card/glare-card.vue(2 hunks)
🔇 Additional comments (3)
docs/.vitepress/theme/glare-card/glare-card.vue (3)
33-33: LGTM! Direct model assignment correctly triggers cleanup.The change from
setCard(false)tomodel = falseproperly leverages Vue's reactivity to trigger the watch callback that removes the overflow classes.
78-78: LGTM! Import supports the new watch-based cleanup.
82-82: LGTM! Default value correctly initializes to closed state.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
docs/.vitepress/theme/glare-card/glare-card.vue (1)
84-101: SSR guard resolves the critical issue; consider optional refactor to reduce duplication.The
typeof window === 'undefined'guards at lines 85 and 97 correctly prevent SSR build errors, resolving the critical concern from the previous review. The logic correctly locks/unlocks overflow when the modal opens/closes and cleans up on unmount.However, the class manipulation logic is duplicated between the watch and unmount hook. Per the previous suggestion, extracting this into a helper function would improve maintainability and allow for a defensive
document.bodycheck:+const toggleOverflow = (value: boolean) => { + if (typeof window === 'undefined') return + + const { documentElement, body } = document + if (!body) return + + if (value) { + documentElement.classList.add('overflow-hidden') + body.classList.add('overflow-hidden') + } else { + documentElement.classList.remove('overflow-hidden') + body.classList.remove('overflow-hidden') + } +} + -watch(model, (value) => { - if (typeof window === 'undefined') return - - if (value) { - document.documentElement.classList.add('overflow-hidden') - document.body.classList.add('overflow-hidden') - } else { - document.documentElement.classList.remove('overflow-hidden') - document.body.classList.remove('overflow-hidden') - } -}, { immediate: true }) +watch(model, toggleOverflow, { immediate: true }) onBeforeUnmount(() => { - if (typeof window === 'undefined') return - - document.documentElement.classList.remove('overflow-hidden') - document.body.classList.remove('overflow-hidden') + toggleOverflow(false) })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/.vitepress/theme/glare-card/glare-card.vue(2 hunks)
🔇 Additional comments (2)
docs/.vitepress/theme/glare-card/glare-card.vue (2)
33-33: LGTM!The change from
setCard(false)tomodel = falseis more idiomatic and directly leverages Vue's reactivity system.
78-82: LGTM!The imports and model initialization are correct. Using
defineModel({ default: false })properly establishes the default closed state for the modal.
1 participant
Previously, after dismissing the glare card (Nice kirakira card BTW! 😉), the
overflow-hiddenonbodywas not cleaned up, which made the page not scrollable. This pull request tries to fix that issue.Summary by CodeRabbit
Bug Fixes
Refactor